[water] promote the duplicate non-unit step verifier#1050
Merged
Conversation
Until now, we have only been verifying the absence of a second non-unit step in index expressions of read and write operations. Do so for every operation. The check is performed when verifying the hyperparameters attribute since hyperparameters are likely necessary to evaluate the step expression. Therefore, the verification will be skipped in absence of this attribute. This is not ideal, but the alternative is to have every single verifier look up in the region tree to find the hyperparameters, which isn't very efficient. A better, longer-term solution is to introduce a top-level wave kernel operation where hyperparameters are mandatory. Closes #1013. Signed-off-by: Alex Zinenko <git@ozinenko.com>
ftynse
changed the title
ftynse
requested review from
Copilot and
martin-luecke
and removed request for
Copilot
Contributor
There was a problem hiding this comment.
Pull request overview
This PR promotes the duplicate non-unit step verifier for index expressions from being specific to read/write operations to applying to all Wave operations with an index attribute. Previously, the multi-step check was embedded in verifyIndexElementsPerThread (called only for read/write/selfIndex ops). Now it's centralized in a new verifyIndexStepsAtMostOneNonUnit function in WaveDialect.cpp, which runs for all ops during the wave.hyperparameters attribute verification.
Changes:
- New
verifyIndexStepsAtMostOneNonUnitfunction inWaveDialect.cppchecks all ops for multiple non-unit index steps (using hyperparameters). Also improves error messages by reporting the symbolic dimension name instead of its integer index. - Removed the inline multi-step check from
verifyIndexElementsPerThreadinWaveOps.cpp, which also drops the now-unused loop index variable. - Tests updated in
ops-invalid.mlirandpropagate-elements-per-thread.mlirto add hyperparameters where needed, update expected error messages, and add a new test case forwave.register.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
water/lib/Dialect/Wave/IR/WaveDialect.cpp |
Adds new verifyIndexStepsAtMostOneNonUnit function and invokes it for all ops during hyperparameter attribute verification |
water/lib/Dialect/Wave/IR/WaveOps.cpp |
Removes the duplicated multi-step check from verifyIndexElementsPerThread; replaces enumerate loop with simpler range loop |
water/test/Dialect/Wave/ops-invalid.mlir |
Updates read_index_multi_step and read_index_multi_step_eval tests for new error message format; adds write_index_multi_step_eval test |
water/test/Dialect/Wave/propagate-elements-per-thread.mlir |
Adds @multiple_non_unit_steps test; updates @index_multi_non_unit_step to remove now-unnecessary hyperparameters |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This is slightly more expensive since we need to look up the hyperparameters every time, but allows to check more reliably and in absence of hyperparameters. If this is slow, will revise via analyses and normal forms. Signed-off-by: Alex Zinenko <git@ozinenko.com>
Signed-off-by: Alex Zinenko <git@ozinenko.com>
Contributor
Author
|
Flaky igemm test, we should consider disabling it... |
xintin
pushed a commit
that referenced
this pull request
Mar 6, 2026
Until now, we have only been verifying the absence of a second non-unit step in index expressions of read and write operations. Do so for every operation in the trait that attaches the attribute. This is not super-efficient as it requires looking up the attribute on the same parent from all operations, but guarantees the check to happen unlike using the attribute verifier which will not kick in in absence of the hyperparameters attribute even if we can see a problem. A better, longer-term solution is to introduce a top-level wave kernel operation where hyperparameters are mandatory. We can also go for a normal form that will perform a top-down verification collecting the attributes on the way. Closes #1013. --------- Signed-off-by: Alex Zinenko <git@ozinenko.com>
xintin
pushed a commit
that referenced
this pull request
Mar 6, 2026
Until now, we have only been verifying the absence of a second non-unit step in index expressions of read and write operations. Do so for every operation in the trait that attaches the attribute. This is not super-efficient as it requires looking up the attribute on the same parent from all operations, but guarantees the check to happen unlike using the attribute verifier which will not kick in in absence of the hyperparameters attribute even if we can see a problem. A better, longer-term solution is to introduce a top-level wave kernel operation where hyperparameters are mandatory. We can also go for a normal form that will perform a top-down verification collecting the attributes on the way. Closes #1013. --------- Signed-off-by: Alex Zinenko <git@ozinenko.com> Signed-off-by: xintin <gaurav.verma@amd.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Until now, we have only been verifying the absence of a second non-unit
step in index expressions of read and write operations. Do so for every
operation in the trait that attaches the attribute. This is not super-efficient
as it requires looking up the attribute on the same parent from all
operations, but guarantees the check to happen unlike using the attribute
verifier which will not kick in in absence of the hyperparameters attribute
even if we can see a problem. A better, longer-term solution is to
introduce a top-level wave kernel operation where hyperparameters are
mandatory. We can also go for a normal form that will perform a
top-down verification collecting the attributes on the way.
Closes #1013.